fix(lookup): skip empty cells in approximate MATCH/VLOOKUP/HLOOKUP/XLOOKUP (HF-223)#1697
Open
marcin-kordas-hoc wants to merge 3 commits into
Open
fix(lookup): skip empty cells in approximate MATCH/VLOOKUP/HLOOKUP/XLOOKUP (HF-223)#1697marcin-kordas-hoc wants to merge 3 commits into
marcin-kordas-hoc wants to merge 3 commits into
Conversation
✅ Deploy Preview for hyperformula-dev-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c79e10c. Configure here.
Performance comparison of head (dde3c8a) vs base (72205bd) |
…223) Excel and Google Sheets ignore genuinely empty cells (but not empty strings) when computing the lower/upper bound for an approximate match. HyperFormula instead landed on an empty cell during binary search and returned #N/A (its EmptyValue Symbol never matched the key), or, in descending mode, reported the wrong position. findLastOccurrenceInOrderedRange now runs the ordered search over a compacted list of non-empty cell indices and maps the result back to the original index space, so empty cells keep their slots and the matched non-empty cell's original 1-based position is reported unchanged. When the range has no non-empty cells the function returns NOT_FOUND, so an all-empty range yields #N/A for every direction/bound instead of falling through to the offset-0 branches. The in-memory ordered path in AdvancedFind.findNormalizedValue skips EmptyValue for the same reason. Empty strings are unaffected (text is ranked above numbers, so they still terminate a numeric run). Exact match (matchType 0) is untouched. Shared by approximate MATCH(+/-1), sorted VLOOKUP/HLOOKUP, and XLOOKUP(searchMode +/-2). Tests for the public test suite are tracked separately in handsontable/hyperformula-tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c79e10c to
ed10ce5
Compare
…e JSDoc (HF-223) Address prep-flip review notes: explain why the empty-cell compaction is O(n) (correctness requires it; empties make the predicate non-monotonic) and add a JSDoc block to AdvancedFind.findNormalizedValue for family consistency with findLastOccurrenceInOrderedRange. Comment/JSDoc only — no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sequba
requested changes
Jun 25, 2026
…HF-223) Per review: the empty-skipping compaction must apply to approximate (bound) lookups only. Run exact-match mode (ifNoMatch === 'returnNotFound') directly over the original range so it keeps its O(log n) guarantee and its pre-empty-skip behaviour — an exact search neither matches a blank nor is redirected past one. CHANGELOG reworded to the house style (PR link). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Task linked: HF-223 Fix MATCH incompatibility with Excel |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1697 +/- ##
===========================================
- Coverage 97.16% 97.15% -0.01%
===========================================
Files 176 176
Lines 15322 15334 +12
Branches 3387 3394 +7
===========================================
+ Hits 14887 14898 +11
- Misses 435 436 +1
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What & why
Approximate
MATCH,VLOOKUP,HLOOKUPandXLOOKUPreturned#N/A(or, in descending mode, the wrong position) when the sorted search range contained genuinely empty cells. Excel 2021 and Google Sheets skip empty cells when computing the lower/upper bound; HyperFormula instead landed on an empty cell during binary search (itsEmptyValueSymbol never matched the key). [HF-223]Root cause
findLastOccurrenceInOrderedRange(src/interpreter/binarySearch.ts):compare()ranksEmptyValuebelow every value, breaking the sort invariant the binary search relies on, and thetypeof foundValue !== typeof searchKeyguard then turned a landing-on-empty into#N/A. Shared by approximateMATCH(±1), sortedVLOOKUP/HLOOKUP, andXLOOKUP(searchMode ±2).Fix
AdvancedFind.findNormalizedValueskipsEmptyValueon its in-memory ordered path for the same reason.Performance & edge cases
O(n)over the search range, trading away binary search'sO(log n)guarantee. This is required for correctness — with empty cells interspersed the search predicate is no longer monotonic, so the binary search cannot run directly on the original range (rationale documented inline inbinarySearch.ts). A future optimization can keepO(log n)when the range is statically known to be empty-free.NOT_FOUNDdirectly instead of falling through to the lower/upper-bound edge-case logic (which could otherwise return offset0).foundIndex + 1, so skipped empty slots never shift the reported position.Excel / Google Sheets parity
Behaviour verified against the latest Excel and Google Sheets, including exact-vs-blank, empty-string-not-skipped, and the descending early-stop case.
Tests
Public test suite: handsontable/hyperformula-tests#17 (matching branch
task/hf-223-match-empty-cells).Definition of Done
Note
Medium Risk
Touches shared ordered-search logic used by multiple lookup functions; behavior change is intentional but could affect formulas that relied on the old wrong results. Exact-match path is explicitly isolated to reduce regression risk.
Overview
Fixes approximate
MATCH,VLOOKUP,HLOOKUP, andXLOOKUPwhen the sorted search range has genuinely empty cells—they no longer return#N/Aor the wrong position because binary search treatedEmptyValueas ordered data.findLastOccurrenceInOrderedRangenow skips blanks only for lower/upper-bound modes: it builds a compact list of non-empty indices, runs the search there, and maps the result back so reported positions still match the original range. Exact match (returnNotFound) stays on the original range with prior semantics. All-empty ranges return not found; stepping to the “next” bound uses the next non-empty index, notfoundIndex + 1.AdvancedFind.findNormalizedValueapplies the same empty-cell skip on its in-memory approximate path. Empty strings are unchanged; changelog documents the fix ([#1697]).Reviewed by Cursor Bugbot for commit dde3c8a. Bugbot is set up for automated code reviews on this repo. Configure here.